Make missing-Redis-state job failures loud and self-diagnosing#1241
Make missing-Redis-state job failures loud and self-diagnosing#1241mihow wants to merge 9 commits into
Conversation
✅ Deploy Preview for antenna-ssec canceled.
|
|
Warning Review limit reached
More reviews will be available in 26 minutes and 3 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRedis-missing-state diagnostics now flow from the ML state manager into job-task logging and missing-state failure handling for both pipeline stages. Failure reasons are now written into progress errors, and tests cover the new diagnostics and log strings. ChangesRedis missing-state diagnostics and failure handling
Sequence Diagram(s)sequenceDiagram
participant process_nats_pipeline_result
participant AsyncJobStateManager
participant NATS
participant _fail_job
process_nats_pipeline_result->>AsyncJobStateManager: update_state(stage)
AsyncJobStateManager-->>process_nats_pipeline_result: None + diagnose_missing_state()
process_nats_pipeline_result->>NATS: ack()
process_nats_pipeline_result->>_fail_job: fail job with stage-specific reason
_fail_job-->>process_nats_pipeline_result: set FAILURE
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for antenna-preview canceled.
|
6730cfb to
111d5d6
Compare
There was a problem hiding this comment.
Pull request overview
Improves debuggability of async ML job failures when Redis-backed job state is missing by adding Redis-target diagnostics, propagating the failure reason into Job.progress.errors, and updating tests to lock in the new behavior.
Changes:
- Add
AsyncJobStateManager.diagnose_missing_state()and log a diagnostic snapshot on the missing-state path. - Include stage-specific missing-state diagnostics in
_fail_jobreasons and persist those reasons intoprogress.errorsfor UI visibility. - Add/adjust unit tests covering diagnostics output and
_fail_jobpersistence semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ami/ml/orchestration/async_job_state.py |
Adds missing-state diagnostic helper and warning log in update_state. |
ami/jobs/tasks.py |
Logs Redis target at job start; passes diagnostics into _fail_job; appends reason into progress.errors. |
ami/ml/tests.py |
Adds tests for diagnose_missing_state() output in “never initialized” and “partial keys present” cases. |
ami/jobs/tests/test_tasks.py |
Updates missing-state reason assertions; adds regression tests for _fail_job writing progress.errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
111d5d6 to
0c1dbc1
Compare
Before: when process_nats_pipeline_result found the job's total-images
key missing, it failed the job with the hardcoded reason
"Job state keys not found in Redis (likely cleaned up concurrently)".
The reason string went to job.logger only — not progress.errors — and
collapsed three distinct causes (DB-index drift across hosts, key
eviction, never-initialized state) into one misleading line. The Redis
target (host:port/db) was never logged, so operators couldn't tell a
cache-DB split from a cleanup race without shelling into workers.
This commit makes that path name the actual cause:
1. AsyncJobStateManager.diagnose_missing_state() returns a one-line
snapshot: masked host:port, DB index, and SCAN output for job:{id}:*
with SCARDs. "keys_for_job=<none>" ⇒ never initialized or wrong DB;
SCARDs present ⇒ partial cleanup / eviction.
2. update_state() emits a WARN with that snapshot immediately before
returning None, so the trigger shows up in the worker log even if
the caller's FAILURE log is filtered out.
3. process_nats_pipeline_result passes the live snapshot to _fail_job
instead of the hardcoded string.
4. _fail_job appends the reason to job.progress.errors before save, so
the UI surfaces FAILUREs with a cause instead of errors=[].
5. run_job logs "Running job X on redis=HOST:PORT/dbN" at start. Cross-
host DB drift becomes visible in every job's log without needing to
already suspect it.
Tests added:
- diagnose_missing_state with never-initialized state (keys_for_job=<none>)
- diagnose_missing_state after partial cleanup (SCARDs of surviving sets
listed)
- Existing "genuinely missing state" test assertion updated to match the
richer reason string.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add TestFailJob with two TDD-confirmed cases: - ``test_fail_job_appends_reason_to_progress_errors`` — verifies the reason string lands in ``job.progress.errors`` (both in-memory and after refresh_from_db) so UI surfaces the cause of the FAILURE. The existing ``_fail_job`` call-site tests in ``TestProcessNatsPipelineResultError`` mock ``_fail_job`` entirely, so a regression that stops appending to ``progress.errors`` would slip through undetected. - ``test_fail_job_is_noop_on_already_final_job`` — regression guard for the early-return branch when the job is already in a final state. Also: - Comment the bare ``except Exception: pass`` around the ``progress.errors.append`` in ``_fail_job`` to explain why we swallow diagnostic-write failures. - Extend the ``AsyncJobStateManager.diagnose_missing_state`` docstring with a one-paragraph note about the SCAN cost (failure-path only, per-job fanout of at most four keys) to head off the obvious review question. Co-Authored-By: Claude <noreply@anthropic.com>
Fix three inaccurate comments flagged in review: - diagnose_missing_state() is called from update_state and the result handler (not _fail_job); note it can run at most twice per FAILURE. - SCAN is O(keyspace) regardless of MATCH; acceptable only because it runs on the rare missing-state failure path. - _describe_redis_target() returns a redis=... prefixed string.
… duplicate rationale [skip ci]
- async_job_state.py update_state: "previously all surfaced as..." → states the
contract ("Distinguishes three causes that map to the same symptom")
- async_job_state.py diagnose_missing_state: remove inline SCAN/MATCH cost
duplicate that repeated the docstring verbatim; keep the docstring
- tasks.py _fail_job: "Previously the reason lived only in job.logger..." →
states what the code does ("Operators can see the cause in the job detail view")
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…peline_result Adds test_genuinely_missing_state_results_stage_acks_and_fails_job to TestProcessNatsPipelineResultError, mirroring the existing process-branch test for the results-stage missing-state path (tasks.py lines 378-388). Verifies that when update_state returns None at stage=results, the task acks NATS (to stop redelivery) and calls _fail_job with a reason string containing "stage=results". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
diagnose_missing_state() embedded the Redis host:port in the string that becomes
the job's public progress.errors / failure reason, leaking the internal Redis
hostname into a user-visible surface. The DB index is the load-bearing diagnostic
(the missing-state incidents were a db0-vs-db1 mismatch across processes), so the
public string now reports only "redis db{N}: keys_for_job=...". The host:port
moves to a new connection_target() used solely in the server-side warning logged
by update_state, where operators see it without it reaching the public job log.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The earlier fix sanitized diagnose_missing_state()/progress.errors but missed the other public surface: run_job's start log line `Running job ... on redis=host:port/dbN` goes through job.logger into the JobLog table, which the UI shows — so the internal Redis host leaked into every job's public log on the success path, not just failures. Split the helpers: _redis_db_index() returns only the DB index for the public job log (the load-bearing signal for cross-host DB-drift), and _describe_redis_target() (host:port) now logs to the server logger only. Also stop _fail_job silently swallowing a failed progress.errors append — log a warning so the swallow is observable — and correct the run_job comment that overstated the DB-0-vs-DB-1 convention. Adds TestRedisTargetLogging pinning that the public string omits host:port while the server string keeps it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cut the over-long diagnose_missing_state docstring (dropped the SCAN-cost and standalone defensive paragraphs) and the run_job DB-index comment, and removed changelog-style references to the prior hardcoded reason string. Behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f2dd114 to
a8e9c7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ami/jobs/tasks.py (1)
446-447: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize
_redis_db_index()error output before writing to public job logs.Line 446/447 currently includes raw exception text, which can include
host:portand reintroduce infrastructure leakage injob.logger.info(...).🔧 Suggested fix
def _redis_db_index() -> str: @@ - except Exception as e: - return f"(unavailable: {e})" + except Exception: + # Keep public job logs infrastructure-safe. + return "(unavailable)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ami/jobs/tasks.py` around lines 446 - 447, The `_redis_db_index()` exception handling currently returns raw exception text, which can leak infrastructure details into public job logs. Update the `except Exception as e` branch to sanitize the error output before formatting it into the returned string, and keep the public-facing message generic in the `job.logger.info(...)` path so no host, port, or other internal connection details are exposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ami/jobs/tasks.py`:
- Around line 480-488: The current `progress.errors.append(...)` flow in
`tasks.py` does an in-memory JSONB mutation and then saves the whole `progress`
object, which can clobber concurrent sub-field updates. Update the
failure-reason path around the `job.progress.errors` handling to use an atomic
server-side JSONB update or an optimistic compare-and-swap for
`progress.errors`, while keeping the existing `JobState.FAILURE` and
`job.save(...)` finalization logic intact.
In `@ami/ml/orchestration/async_job_state.py`:
- Around line 229-230: The fallback diagnostics in async_job_state.py are
exposing raw exception text from the except block, which can leak Redis
host:port details into user-visible progress.errors. Update the diagnostic
fallback in the relevant helper in AsyncJobState to avoid returning the raw
exception string from e; instead return a sanitized generic message that
preserves failure context without endpoint information, and ensure any call
sites using this value continue to surface only the redacted text.
---
Duplicate comments:
In `@ami/jobs/tasks.py`:
- Around line 446-447: The `_redis_db_index()` exception handling currently
returns raw exception text, which can leak infrastructure details into public
job logs. Update the `except Exception as e` branch to sanitize the error output
before formatting it into the returned string, and keep the public-facing
message generic in the `job.logger.info(...)` path so no host, port, or other
internal connection details are exposed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc9e29ac-3426-45ca-bf30-f8d3ad3dd719
📒 Files selected for processing (4)
ami/jobs/tasks.pyami/jobs/tests/test_tasks.pyami/ml/orchestration/async_job_state.pyami/ml/tests.py
| job.progress.errors.append(reason) | ||
| except Exception as e: | ||
| # Don't let a diagnostic-write failure mask the original FAILURE, but record | ||
| # that the reason could not be attached so the swallow is observable — otherwise | ||
| # the UI silently loses the cause this PR exists to surface. | ||
| logger.warning("Job %s: could not append failure reason to progress.errors: %s", job_id, e) | ||
| job.update_status(JobState.FAILURE, save=False) | ||
| job.finished_at = datetime.datetime.now() | ||
| job.save(update_fields=["status", "progress", "finished_at"]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
progress.errors.append(...) is still vulnerable to JSONB write clobber under concurrent writers.
Line 480 + Line 488 performs an in-memory append then writes the whole progress blob; concurrent updates to other progress sub-fields can overwrite this reason (or be overwritten), defeating persistence reliability.
Based on learnings: jobs_job.progress is JSONB, and save(update_fields=["progress"]) is not safe for concurrent sub-field mutation; use server-side atomic JSONB update (or optimistic CAS) for progress.errors.
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 481-481: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ami/jobs/tasks.py` around lines 480 - 488, The current
`progress.errors.append(...)` flow in `tasks.py` does an in-memory JSONB
mutation and then saves the whole `progress` object, which can clobber
concurrent sub-field updates. Update the failure-reason path around the
`job.progress.errors` handling to use an atomic server-side JSONB update or an
optimistic compare-and-swap for `progress.errors`, while keeping the existing
`JobState.FAILURE` and `job.save(...)` finalization logic intact.
Source: Learnings
| except Exception as e: | ||
| return f"(diagnostics failed: {e})" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize fallback diagnostics to avoid leaking Redis endpoint details.
Line 229 returns the raw exception string in a value that is surfaced to user-visible progress.errors; Redis exceptions can embed host:port, which breaks the no-host-leak contract.
Proposed fix
- except Exception as e:
- return f"(diagnostics failed: {e})"
+ except Exception:
+ return "redis db?: keys_for_job=<diagnostics_failed>"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| return f"(diagnostics failed: {e})" | |
| except Exception: | |
| return "redis db?: keys_for_job=<diagnostics_failed>" |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 229-229: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ami/ml/orchestration/async_job_state.py` around lines 229 - 230, The fallback
diagnostics in async_job_state.py are exposing raw exception text from the
except block, which can leak Redis host:port details into user-visible
progress.errors. Update the diagnostic fallback in the relevant helper in
AsyncJobState to avoid returning the raw exception string from e; instead return
a sanitized generic message that preserves failure context without endpoint
information, and ensure any call sites using this value continue to surface only
the redacted text.
… guards [skip ci] The missing-state diagnostics are a low-frequency logging path; the two diagnose_missing_state content assertions and the _fail_job progress.errors append test were disproportionate coverage for it. Kept the security-relevant guards (TestRedisTargetLogging and test_diagnose_missing_state_omits_host_*) that pin the Redis host out of user-facing strings, plus the missing-state ack+fail behavior tests. No production code changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I don't see a reason to continue pursuing this PR until we see the issue again. |
Summary
When an async ML job's Redis state goes missing, the job is failed — but that failure used to be close to silent and actively misleading. The reason string was hardcoded ("Job state keys not found in Redis (likely cleaned up concurrently)"), lived only in the worker log, and never reached the job's
progress.errors, so the UI showed a FAILURE with no cause. Three very different problems all produced that one line:run_jobwrites state to one DB andprocess_nats_pipeline_resultlooks for it in another.This PR keeps the same failure path but makes it name the actual cause and surface it where operators and users will see it. It is observability only — it does not prevent the failure (the underlying misconfig is an infra fix); it makes the next occurrence diagnosable from the first failed job.
It complements #1343 (which already logs job status + age on the missing-state path to tell "late result for a finished job" from "real anomaly"); this PR adds the Redis snapshot as the failure reason and surfaces it in the UI.
List of Changes
process_nats_pipeline_resultpasses a livediagnose_missing_state()snapshot to_fail_job(stage-annotated);_fail_jobappends the reason toprogress.errors.run_joblogs the Redis DB index at task start.update_stateemits aWARNwith the snapshot immediately before returningNone._redis_db_index()/diagnose_missing_state()report the DB index only (safe for the public job log andprogress.errors);_describe_redis_target()/connection_target()report host:port and go to server-side logs only.The DB index is the load-bearing signal for the drift case and is safe to expose; the Redis host names internal infrastructure and is deliberately kept out of anything user-facing.
Tests
ami/jobs/tests/test_tasks.pyandami/ml/tests.py::TestTaskStateManager:diagnose_missing_state()returnskeys_for_job=<none>when state was never initialized, and lists the surviving per-key SCARDs when only the total key is wiped.TestRedisTargetLogging/TestTaskStateManagerassert the public strings (_redis_db_index(),diagnose_missing_state()) expose only the DB index, while the operator-onlyconnection_target()retains host:port — guarding the public/server split._fail_jobappends the reason toprogress.errors(verified in memory and afterrefresh_from_db()), and is a no-op on an already-final job. The existing call-site tests mock_fail_jobentirely, so without these a silent regression on the append would not be caught.Run locally against the rebased branch (dockerized Django, isolated DB):
ami/jobs/tests/test_tasks.py+ami/ml/tests.py::TestTaskStateManagerpass.Not yet exercised end-to-end: the live missing-state FAILURE path (the loud log + the UI reason) is covered by unit tests, not by a deployed run. To verify in a dev env: start an async_api job,
DELitspending_images_totalkey mid-run, and confirm the FAILURE shows the diagnostic in both the UIprogress.errorsand the worker log, and thatrun_job's opening line reports the Redis DB index.Known minor / follow-up
On the
process_nats_pipeline_resultmissing-state path the failure is now reported up to three times —update_state's WARN, #1343's_log_missing_state_context, and the_fail_jobreason — anddiagnose_missing_state()runs twice (once for the WARN, once for the reason). This is the failure path only, so the cost is negligible, but it's a candidate for a small cleanup.Relationship to merged work
progress.errorsvia_fail_job. With both merged there are now two append sites, so a small shared_append_progress_error(job, reason)helper is a reasonable follow-up._fail_job); this PR relies on that contract. fix(jobs): prevent jobs from hanging in STARTED state with no progress #1234 (merged) — STARTED-hang guard; same_fail_job, different entry point.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes